-
Notifications
You must be signed in to change notification settings - Fork 165
Use Create Account Checked & Create Account with Seed Checked #204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
sonicfromnewyoke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice functionality, gives more safety
what do you think about one/two extra calls instead of the full copy/pasted file like so?
#[inline(always)]
pub fn invoke_signed_checked(&self, rent_account_info: &AccountInfo, signers: &[Signer]) -> ProgramResult {
// getting lamports from rent
let rent = Rent::from_account_info(rent_account_info?;
let lamports = rent.minimum_balance(self.space as usize);
// checking if the funding account has enough lamports
if self.from.lamports() < lamports {
return Err(ProgramError::InsufficientFunds);
}
self.invoke_signed(signers)
}074af97 to
2195ba5
Compare
Since we are making the |
my point was in not copy/pasting the whole create_account.rs file just for adding one field in the struct to perform the check. this is more DRY to add the wrapper for |
|
Have you consider adding a "constructor" method instead? This would minimize the code duplication. pub fn with_rent_account(
from: &AccountInfo,
to: &AccountInfo,
rent: &AccountInfo,
space: u64,
owner: &Pubkey,
) -> Result<Self, ProgramError>; |
Please have a look at c709b11 |
| if from.lamports() < lamports { | ||
| return Err(ProgramError::InsufficientFunds); | ||
| } | ||
|
|
||
| if !to.data_is_empty() { | ||
| return Err(ProgramError::InvalidAccountData); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not too sure if we need this checks – the CPI will do this validation and raised the appropriate error.
| if seed.len() > MAX_SEED_LEN { | ||
| return Err(ProgramError::InvalidInstructionData); | ||
| } | ||
|
|
||
| if from.lamports() < lamports { | ||
| return Err(ProgramError::InsufficientFunds); | ||
| } | ||
|
|
||
| if !to.data_is_empty() { | ||
| return Err(ProgramError::InvalidAccountData); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, might be better to remove this checks.
febo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this version than the previous. One point is whether we need the validation or not, since the CPI will do that. Can you measure the difference is CUs? Ideally we would like the "original" version, rent check (no validation), rent check + validation.
Please have a look at: 68cf5be I checked with the pinocchio starter code https://github.com/Nagaprasadvr/solana-pinocchio-starter (
|
Co-authored-by: Fernando Otero <[email protected]>
febo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final nit.
Co-authored-by: Fernando Otero <[email protected]>
febo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
…yz#204) * use Sysvar rent account * implement invoke_signed_checked * removed the checked files * implement with_rent_check constructor * fix imports * remove checks * Apply suggestions from code review Co-authored-by: Fernando Otero <[email protected]> * fix formatting * Apply suggestions from code review Co-authored-by: Fernando Otero <[email protected]> --------- Co-authored-by: Fernando Otero <[email protected]>
Instead of passing lamports as part of arguments, passing Sysvar rent account.
So forcing user to pass
sysvar_rent_accountinstead of doing:lamports: Rent::get()?.minimum_balance(<STATE>::LEN)where
Rent::get()makes a sysvar call directly from runtime, which makes it consume more CUs.Besides, by internally computing lamports, the code doesn't allow devs to use excess or less lamports